chore: Check materialized view metrics emitted in tests#27724
Open
Sullivan-Patrick wants to merge 1 commit into
Open
chore: Check materialized view metrics emitted in tests#27724Sullivan-Patrick wants to merge 1 commit into
Sullivan-Patrick wants to merge 1 commit into
Conversation
Contributor
Reviewer's GuideAdds a reusable test helper that asserts materialized-view rewrites occurred by checking the OPTIMIZED_WITH_MATERIALIZED_VIEW_SUBQUERY_COUNT runtime metric, migrates existing materialized-view planner tests to use it, adds a new metric-focused test, and performs minor test cleanups/renames and formatting adjustments. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
assertMaterializedViewRewriteOccurred, consider including the executed SQL in the assertion failure messages (in addition to the query id) to make it easier to debug why a materialized view rewrite was not detected. - The helper
assertMaterializedViewRewriteOccurredlooks generally useful beyond this class; consider moving it to a shared base test utility so other materialized view tests can assert on the same runtime metric consistently. - The renamed test
testMaterializedViewOptimizationWithUnsupportedFunctionSubquerystill contains a comment stating thatlength()is now supported as a scalar function and both subqueries should be rewritten, which seems inconsistent with the new method name—clarify whether this test is meant to verify support or lack of support and align the name/comment accordingly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `assertMaterializedViewRewriteOccurred`, consider including the executed SQL in the assertion failure messages (in addition to the query id) to make it easier to debug why a materialized view rewrite was not detected.
- The helper `assertMaterializedViewRewriteOccurred` looks generally useful beyond this class; consider moving it to a shared base test utility so other materialized view tests can assert on the same runtime metric consistently.
- The renamed test `testMaterializedViewOptimizationWithUnsupportedFunctionSubquery` still contains a comment stating that `length()` is now supported as a scalar function and both subqueries should be rewritten, which seems inconsistent with the new method name—clarify whether this test is meant to verify support or lack of support and align the name/comment accordingly.
## Individual Comments
### Comment 1
<location path="presto-hive/src/test/java/com/facebook/presto/hive/TestHiveMaterializedViewLogicalPlanner.java" line_range="2130-2131" />
<code_context>
@Test
- public void testMaterializedViewOptimizationWithScalarFunctionSubquery()
+ public void testMaterializedViewOptimizationWithUnsupportedFunctionSubquery()
{
Session queryOptimizationWithMaterializedView = Session.builder(getSession())
</code_context>
<issue_to_address>
**suggestion (testing):** Clarify test intent regarding "unsupported" vs "supported" function rewrites and consider adding a negative case for truly unsupported functions
The renamed test still asserts that rewrites occur for the `length()` subqueries, so it effectively covers a *supported* function scenario. If you want to keep the new name, please also add a separate test using a truly unsupported function that asserts no rewrite (and no metric increment). Otherwise, consider reverting/adjusting the name to reflect that this test covers supported-function rewrites.
```suggestion
@Test
public void testMaterializedViewOptimizationWithSupportedFunctionSubquery()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sullivan-Patrick
force-pushed
the
mv-rewrite-runtime-metric-test
branch
3 times, most recently
from
27b209a to
32ac08d
Compare
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 8982f13...1b25a2a. No notifications. |
Sullivan-Patrick
force-pushed
the
mv-rewrite-runtime-metric-test
branch
from
32ac08d to
1b25a2a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a test helper
assertMaterializedViewRewriteOccurredthat verifies theoptimizedWithMaterializedViewSubqueryCountruntime metric was incremented during query execution, and migrates existing tests to use it.Motivation and Context
We want to verify rewrites are occurring, this change confirms metrics are emitted during tests in
TestHiveMaterializedViewLogicalPlannerverifying rewrites occurred.Impact
Test Plan
Tests passing with new changes
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Add a test helper to assert that materialized view rewrites occurred and update materialized view planner tests to validate runtime metrics for rewrites.
Tests: